Compactor: remove malformed blocks after delay #1053

mjd95 · 2019-04-18T16:08:38Z

Changes

Introduce a consistency delay in the compactor. If the compactor sees a block that does not have a meta.json in its initial sync, and the block is older than consistencyDelay, then the compactor removes it.

Related issues #621 #976

Verification

Manually deleted the meta.json files from some blocks in our testing bucket, ran this compactor against them and checked that it deleted the blocks in its inital sync then continued with the rest of the compaction.

bwplotka

This is great and exactly what we need, but we need to improve it with 2 things:

Reuse with syncDelay we already have.
Add support for the same thing in store gateway! As partial blocks affects store as well.

Thanks for this @mjd95

pkg/compact/compact.go

pkg/block/block.go

mjd95 · 2019-04-24T13:50:52Z

Have changed around to re-use syncDelay. It has a default value of 30 minutes which I think is good. The only danger I guess would be if people have been running the compactor with a very low value? I wonder if we should still keep a safety check in removeIfMalformed anyway?

Discussed offline and won't add anything to the store gateway right now, as it's good to keep the store as read only.

bwplotka

Mostly allright, some suggestions though.

pkg/block/block.go

cmd/thanos/compact.go

pkg/compact/compact.go

bwplotka · 2019-04-25T17:08:54Z

Have changed around to re-use syncDelay. It has a default value of 30 minutes which I think is good. The only danger I guess would be if people have been running the compactor with a very low value? I wonder if we should still keep a safety check in removeIfMalformed anyway?

Good point. I think we need to define some minimumDelay, Let's imagine you run strongly consistent objstorage like GCS. Even in this case consistencyDelay=0s is wrong as sidecars can upload blocks slowly, so it may take some time where the block is partially uploaded and we don't want compactor to remove that, right?

mjd95 · 2019-04-25T17:23:49Z

Yeah @bwplotka that's the sort of thing I was worried about. Now that the --sync-delay flag is also controlling whether the block is old enough to be deleted or not, we need to be more careful if the flag is set very low.

I think we should probably have some sort of extra safety check in removeIfMetaMalformed. Either we could force the --sync-delay flag value to be >10m, or we could just add in an additional check removeIfMetaMalformed for some delay Thanos itself sets. I think I prefer the latter.

(I changed the flag name BTW. If we land it like this, it's annoying that the name change is a breaking changed, but it does help to show that the behaviour of the flag has changed.)

bwplotka · 2019-04-25T17:31:20Z

Yes, I think the flag name here is reasonable and it's not heavy used I think. Not sure for the the constraint. I think syncDelay delay minimum value makes sense more

bwplotka · 2019-04-26T17:15:11Z

Okey, let's not fit this into release although I am super sad ): I think this is critial piece. If you have 0 consistency delay it means you will kill your sidecar uploads ): Not fun. No way to separate those from bucket perspecitve as all is in meta. So this means some sort of consistency delay is must have, even for tests.

Alternatives:

Push meta.json first and have another special file for locking (we would rather have native block support)
Separate blocks to tell which ones are from compactor? Maybe compacted block should have some indication in dir name? or special file?
Maybe we should have consistency Delay being configured but have special removeIfMalformed after hardcoded 30 min?

bwplotka · 2019-05-02T23:09:16Z

pkg/compact/compact.go

@@ -194,6 +198,9 @@ func (c *Syncer) syncMetas(ctx context.Context) error {
 					continue
 				}
 				if err != nil {
+					if removedOrIgnored := c.removeIfMetaMalformed(workCtx, id); removedOrIgnored {


Well technically you can do just if c.removeIfMetaMalformed(workCtx, id) here (:

bwplotka

Let's ship it! LGTM.

xjewer · 2019-05-06T21:59:09Z

why it removes data by default, can we add a flag to explicitly do removal?

midnightconman · 2019-05-07T01:18:58Z

why it removes data by default, can we add a flag to explicitly do removal?

Like a debug or dry-run flag?

xjewer · 2019-05-08T16:44:38Z

Like a debug or dry-run flag?

the opposite, the flag which explicitly tells to remove these blocks, like --compact.remove-malformed-blocks

bwplotka · 2019-05-08T21:26:09Z

Very good point @xjewer, we could do that for safety, but we cannot see any current case when we remove important block, not duplicated in other places (even with eventually consistent storage), so why would you not remove the paritaly uploaded blocks (no meta) automatically? If you are worried I am happy to add such flag.

xjewer · 2019-05-09T09:30:29Z

while it's happening #318 (comment), i'd prefer removing manually instead of losing data, e.g. if I a meta.json is missing, I can get it at least from debug directory

bwplotka · 2019-05-09T13:43:33Z

I cannot see the use case but I am fine with manual option. I would argue if manual should be default though.

Even the issue you mentioned - I doubt it is causing partial uploads as we remove the partial uploaded one immdiately after failure (see: https://github.com/improbable-eng/thanos/blob/e6d5b49bf2d2281f8f2f2d0e853158174a2e541f/pkg/block/block.go#L113) and we do not proceed with removal of old compacted blocks.

xjewer · 2019-05-09T14:04:17Z

I doubt it is causing partial uploads as we remove the partial uploaded one immdiately after failure

for some reason, there's a 400 error on proxy, but minio (seems to be multipart upload ¯\_(ツ)_/¯) doesn't treat it as an error, thus it uploads block with no error and as a consequence there's no cleanUp.

we do not proceed with removal of old compacted blocks.

wait, I see the opposite

Malformed blocks older than the maximum of consistency-delay and %s will be removed

https://github.com/improbable-eng/thanos/pull/1053/files#diff-921a1164068d3a04217b2d85714b16adR298

bwplotka · 2019-05-09T14:50:50Z

I meant "we do not proceed with removal of old compacted blocks." if the upload was failed (:

bwplotka · 2019-05-09T14:51:53Z

@xjewer let's jump on slack I am confused (:

for some reason, there's a 400 error on proxy, but minio (seems to be multipart upload ¯_(ツ)_/¯) doesn't treat it as an error, thus it uploads block with no error and as a consequence there's no cleanUp.

You mean file was NOT uploaded but we get 200 and success from minio Upload request?

* query: cleanup store statuses as they come and go (thanos-io#910) Signed-off-by: Adrien Fillon <adrien.fillon@cdiscount.com> * [docs] Example of using official prometheus helm chart to deploy server with sidecar (thanos-io#1003) * update documentation with an example of using official prometheus helm chart Signed-off-by: Ivan Kiselev <ivan@messagebird.com> * a little formatting to values Signed-off-by: Ivan Kiselev <ivan@messagebird.com> * satisfy PR comments Signed-off-by: Ivan Kiselev <ivan@messagebird.com> * Compact: group concurrency (thanos-io#1010) * compact: add concurrency to group compact * add flag to controll the number of goroutines to use when compacting group * update compact.md for group-compact-concurrency * fixed: miss wg.Add() * address CR * regenerate docs * use err group * fix typo in flag description * handle context * set up workers in main loop * move var initialisation * remove debug log * validate concurrency * move comment * warn -> error * remove extra newline * fix typo * dns: Added miekgdns resolver as a hidden option to query and ruler. (thanos-io#1016) Fixes: thanos-io#1015 Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * query: set default evaluation interval (thanos-io#1028) Subqueries allows request with no [specified resolution](https://prometheus.io/blog/2019/01/28/subquery-support/). Set it up and allow to configure default evaluation interval. * store+compactor: pre-compute index cache during compaction (thanos-io#986) Fixes first part of thanos-io#942 This changes allow to safe some startup & sync time in store gateway as it is no longer is needed to compute index-cache from block index on its own. For compatibility store GW still can do it, but it first checks bucket if there is index-cached uploaded already. In the same time, compactor precomputes the index cache file on every compaction. To allow quicker addition of index cache files we added `--index.generate-missing-cache-file` flag, that if enabled precompute missing files on compactor startup. Note that it will take time and it's only one-off step per bucket. Signed-off-by: Aleksei Semiglazov <xjewer@gmail.com> * Added website for Thanos' docs using Hugo. (thanos-io#807) Hosted in github pages. Signed-off-by: adrien-f <adrien.fillon@gmail.com> Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * gcs: Fixed scopes for inline ServiceAccount option. (thanos-io#1033) Without this that option was unusable. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Fixed root docs and liche is now checking root dir as well. (thanos-io#1040) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * storage docs: add detail about GCS policies and testing (thanos-io#1037) * add more details about GCS policies and testing * remove fixed names from exec command * Prometheus library updated to v2.8.1 (thanos-io#1009) * compact: group concurrency improvements (thanos-io#1029) * group concurrency improvements * remove unnecessary error check * add to wg in main goroutine * receive: Add block shipping (thanos-io#1011) * receive: Add retention flag for local tsdb storage (thanos-io#1046) * querier: Add /api/v1/labels support (thanos-io#905) * Feature: add /api/v1/labels support Signed-off-by: jojohappy <sarahdj0917@gmail.com> * Disabled gossip by default, marked all flags as deprecated. (thanos-io#1055) + changed small label. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * ruler: Fixed Chunk going out or Max Uint16. (thanos-io#1041) Fixes thanos-io#1038 Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * store: azure: allow passing an endpoint parameter for specific regions (thanos-io#980) Fix thanos-io#968 Signed-off-by: Adrien Fillon <adrien.fillon@cdiscount.com> * feature: support POST method for series endpoint (thanos-io#1021) Signed-off-by: Joseph Lee <joseph.t.lee@outlook.com> * bucket verify: repair out of order labels (thanos-io#964) * bucket verify: repair out of order labels * verify repair: correctly order series in the index on rewrite When we have label sets that are not in the correct order, fixing that changes the order of the series in the index. So the index must be rewritten in that new order. This makes this repair tool take up a bunch more memory, but produces blocks that verify correctly. * Fix the TSDB block safe-delete function The directory name must be the block ID name exactly to verify. A temp directory or random name will not work here. * verify repair: fix duplicate chunk detection Pointer/reference logic error was eliminating all chunks for a series in a given TSDB block that wasn't the first chunk. Chunks are now referenced correctly via pointers. * PR feedback: use errors.Errorf() instead of fmt.Errorf() * Use errors.New() Some linters catch errors.Errorf() as its not really part of the errors package. * Liberally comment this for loop We're comparing items by pointers, using Go's range variables is misleading here and we need not fall into the same trap. * Take advantage of sort.Interface This prevents us from having to re-implement label sorting. * PR Feedback: Comments are full sentences. * Cut release 0.4.0-rc.0 (thanos-io#1017) * Cut release 0.4.0-rc.0 🎉 🎉 NOTE: This is last release that has gossip. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> Co-Authored-By: povilasv <p.versockas@gmail.com> * Fixed crossbuild. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * ci: Env fixes. (thanos-io#1058) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Removed bzr requirement for make crossbuild. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Bump github.com/hashicorp/golang-lru from 0.5.0 to 0.5.1 (thanos-io#1051) Bumps [github.com/hashicorp/golang-lru](https://github.com/hashicorp/golang-lru) from 0.5.0 to 0.5.1. - [Release notes](https://github.com/hashicorp/golang-lru/releases) - [Commits](hashicorp/golang-lru@v0.5.0...v0.5.1) Signed-off-by: dependabot[bot] <support@dependabot.com> * Initialze and correctly register all index cache metrics. (thanos-io#1069) * store/cache: add more tests (thanos-io#1071) * Fixed Downsampling process; Fixed `runutil.CloseAndCaptureErr` (thanos-io#1070) * runutil. Simplified CloseWithErrCapture. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Fixed Downsampling process; Fixed runutil.CloseAndCaptureErr Fixes thanos-io#1065 Root cause: * runutil defered capture error function was not passing error properly so unit tests were passing, event though there was bug * streamed block write index cache requires index file which was not closed (saved) properly yet. Closers need to be closed to perform this. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * objstore: Expose S3 region attribute (thanos-io#1060) Minio is able to autodetect the region for cloud providers like AWS but the logic fails with Scaleway Object Storage solution. Related issue on Minio: minio/mc#2570 * Fixed fetching go-bindata failed (thanos-io#1074) * Fixed bug: - fetching go-bindata failed. - change the repo of go-bindata to github.com/go-bindata/go-bindata, because old repo has been archived. - pin the go-bindata as v3.3.1. Signed-off-by: jojohappy <sarahdj0917@gmail.com> * Add CHANGELOG Signed-off-by: jojohappy <sarahdj0917@gmail.com> * Remove CHANGELOG Signed-off-by: jojohappy <sarahdj0917@gmail.com> * add compare flags func to compare flags between prometheus and sidecar (thanos-io#838) Original message: * update documentation for a max/min block duration add compare flags func to compare flags between prom and sidecar * fix some nits Functional change: now we check the configured flags (if possible) and error out if MinTime != MaxTime. We need to check this always since if that is not true then we will get overlapping blocks. Additionally, an error message is printed out if it is not equal to 2h (the recommended value). * Ensured index cache is best effort, refactored tests, validated edge cases. (thanos-io#1073) Fixes thanos-io#651 Current size also includes slice header. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * website: Moved to netlify. (thanos-io#1078) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * website: Fixing netlify. (thanos-io#1080) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * website: Added "founded by" footer. (thanos-io#1081) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * store/proxy: properly check if context has ended (thanos-io#1082) How the code was before it could happen that we might receive some series from the stream however by the time we'd send them back to the reader, it would not read it anymore since the deadline would have been exceeded. Properly use a `select` here to get out of the goroutine if the deadline has been exceeded. Might potentially fix a problem where we see one goroutine hanging constantly (and thus blocking from work being done): ``` goroutine profile: total 126 25 @ 0x42f62f 0x40502b 0x405001 0x404de5 0xe7435b 0x45cc41 0xe7435a github.com/improbable-eng/thanos/pkg/store.startStreamSeriesSet.func1+0x18a /go/src/github.com/improbable-eng/thanos/pkg/store/proxy.go:318 ``` * Cut release v0.4.0-rc.1 (thanos-io#1088) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * website: Removed ghpages handling; fixed docs; and status badge. (thanos-io#1084) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Fix readme (thanos-io#1090) * store: Compose indexCache properly allowing injection for testing purposes. (thanos-io#1098) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * website: add sponsor section on homepage (thanos-io#1062) * website: Adjusted logos sizing and responsiveness. (thanos-io#1105) Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Add Monzo to "Used by" section 🎉 (thanos-io#1106) * Compactor: remove malformed blocks after delay (thanos-io#1053) * compactor removes malformed blocks after delay * compactor removes malformed blocks after delay * include missing file * reuse existing freshness check * fix comment * remove unused var * fix comment * syncDelay -> consistencyDelay * fix comment * update flag description * address cr * fix dupliacte error handling * minimum value for --consistency-delay * update * docs * add test case * move test to inmem bucket * Add Utility Warehouse to "used by" section (thanos-io#1108) * Add Utility Warehouse logo * Make logo smaller * website: add Adform as users (thanos-io#1109) We use Thanos extensively as well so I have added Adform. * Cut release v0.4.0 (thanos-io#1107) Signed-off-by: Bartek Plotka <bwplotka@gmail.com>

Martin Dickson added 3 commits April 17, 2019 15:25

compactor removes malformed blocks after delay

4d9c6ea

compactor removes malformed blocks after delay

f04c1dc

include missing file

4a932af

bwplotka requested review from bwplotka and povilasv April 18, 2019 18:02

bwplotka requested changes Apr 18, 2019

View reviewed changes

pkg/compact/compact.go Outdated Show resolved Hide resolved

pkg/compact/compact.go Outdated Show resolved Hide resolved

bwplotka mentioned this pull request Apr 18, 2019

compactor is in infinite loop when broken block #621

Closed

bwplotka reviewed Apr 23, 2019

View reviewed changes

pkg/block/block.go Outdated Show resolved Hide resolved

Martin Dickson added 7 commits April 24, 2019 14:28

reuse existing freshness check

58dfb04

fix comment

e55e4ac

remove unused var

a1d7c24

fix comment

7d40d91

syncDelay -> consistencyDelay

8794086

fix comment

c33205a

update flag description

16324bc

bwplotka requested changes Apr 24, 2019

View reviewed changes

address cr

a44441a

bwplotka reviewed Apr 25, 2019

View reviewed changes

pkg/compact/compact.go Outdated Show resolved Hide resolved

fix dupliacte error handling

2a3800c

minimum value for --consistency-delay

b94f24f

bwplotka mentioned this pull request Apr 26, 2019

Cut release v0.4.0-rc.1 #1088

Merged

mjd95 added 4 commits May 2, 2019 11:45

update

c7a114f

docs

f75782d

add test case

738ea86

move test to inmem bucket

58febd3

bwplotka reviewed May 2, 2019

View reviewed changes

bwplotka approved these changes May 2, 2019

View reviewed changes

bwplotka merged commit 3a763be into thanos-io:master May 2, 2019

kovszilard mentioned this pull request May 8, 2019

Upgrade prometheus-thanos chart to use thanos 0.4.0 kiwigrid/helm-charts#89

Closed

mjd95 deleted the compactor-removes-malformed-blocks-after-delay branch May 22, 2019 18:42

GiedriusS mentioned this pull request Jul 17, 2019

S3/GCS: Upload/Delete inconsistency (missing chunk file) #1331

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compactor: remove malformed blocks after delay #1053

Compactor: remove malformed blocks after delay #1053

mjd95 commented Apr 18, 2019 •

edited

Loading

bwplotka left a comment

mjd95 commented Apr 24, 2019

bwplotka left a comment

bwplotka commented Apr 25, 2019

mjd95 commented Apr 25, 2019

bwplotka commented Apr 25, 2019

bwplotka commented Apr 26, 2019 •

edited

Loading

bwplotka May 2, 2019

bwplotka left a comment

xjewer commented May 6, 2019

midnightconman commented May 7, 2019

xjewer commented May 8, 2019

bwplotka commented May 8, 2019 •

edited

Loading

xjewer commented May 9, 2019

bwplotka commented May 9, 2019

xjewer commented May 9, 2019

bwplotka commented May 9, 2019

bwplotka commented May 9, 2019

Compactor: remove malformed blocks after delay #1053

Compactor: remove malformed blocks after delay #1053

Conversation

mjd95 commented Apr 18, 2019 • edited Loading

Changes

Verification

bwplotka left a comment

Choose a reason for hiding this comment

mjd95 commented Apr 24, 2019

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka commented Apr 25, 2019

mjd95 commented Apr 25, 2019

bwplotka commented Apr 25, 2019

bwplotka commented Apr 26, 2019 • edited Loading

bwplotka May 2, 2019

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

xjewer commented May 6, 2019

midnightconman commented May 7, 2019

xjewer commented May 8, 2019

bwplotka commented May 8, 2019 • edited Loading

xjewer commented May 9, 2019

bwplotka commented May 9, 2019

xjewer commented May 9, 2019

bwplotka commented May 9, 2019

bwplotka commented May 9, 2019

mjd95 commented Apr 18, 2019 •

edited

Loading

bwplotka commented Apr 26, 2019 •

edited

Loading

bwplotka commented May 8, 2019 •

edited

Loading